Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix app startup issues after changing the args via UI #635

Merged
merged 6 commits into from
Dec 16, 2020
Merged

Fix app startup issues after changing the args via UI #635

merged 6 commits into from
Dec 16, 2020
+9 −7

Conversation

Darkren
Copy link
Contributor

@Darkren Darkren commented Dec 9, 2020 •
edited
Loading

Did you run make format && make check?
Yes
Fixes #627

Changes:

  • Args to the app binary are now taken from the config, not from the previous app start
  • App proc is now correctly removed from the proc manager on app exit, no matter what was the exit reason

How to test this PR:
To test that hypervisor restarts app with the changed args:

  1. Start any app via UI, VPN client for example
  2. Change PK via UI
  3. VPN client should be restarted and connected to the new remote server

To test the original issue:

  1. Modify ./internal/vpn/client.go: put this line return errors.New("fail") to the line around 183rd, right after the dialServeConn and checking its error. This will make the app exit right after connection failure
  2. Build binaries
  3. Modify VPN client config to connect to the remote visor which doesn't have VPN server
  4. Run VPN client from the UI. After a while it will be displayed as being off again, wait for it
  5. Change the remote PK to the visor which has VPN enabled. It should change PK okay
  6. Run VPN client from the UI, you should be using VPN now okay

@Darkren Darkren changed the title [WIP] Pass cmd args from conf, not from the previous app start Fix app startup issues after changing the args via UI Dec 9, 2020
@jdknives jdknives mentioned this pull request Dec 15, 2020
Closed
taras-skycoin
taras-skycoin approved these changes Dec 16, 2020
View reviewed changes
Copy link
Member

@taras-skycoin taras-skycoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

Testing is a bit tricky since you need to switch between two working VPN servers. Please consider it if you are going to test it. You can use these PKs:

036961efd1bdd926e018755ad9cb52f3847191ca776efe412522ffc2781ad6227c
029081ab2f1a9087f7e59b3b58ca2959530335f861725d31aac1b5a3aff150f832

@jdknives
Copy link
Member

@taras-skycoin you tested it? If so we can merge this.

@taras-skycoin
Copy link
Member

Yes, I tested it and left a suggestion for another person who is going to test it too.

@jdknives
Copy link
Member

@taras-skycoin Will merge it then as I tested it already.

@jdknives jdknives merged commit 4233b30 into skycoin:develop Dec 16, 2020
@jdknives jdknives mentioned this pull request Dec 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taras-skycoin taras-skycoin taras-skycoin approved these changes

@jdknives jdknives Awaiting requested review from jdknives

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Darkren @jdknives @taras-skycoin